Skip to content
This repository has been archived by the owner on Jun 13, 2018. It is now read-only.

Problem: inconsistent path treatment leads to confusing behavior. #78

Merged
merged 1 commit into from
Mar 5, 2015

Conversation

evoskuil
Copy link
Member

@evoskuil evoskuil commented Mar 5, 2015

This resolves the various path-related issues identified in #73.

In reviewing a large portion of GSL I discovered a widespread issue pertaining to normalization. Each category of platforms have their own idiosyncrasies with respect to paths. Windows is the most obvious of course, because of the "Gates File System" (as it's referred to in the source) slash reversal.

The problem is that there is an inconsistently-applied canonical form in the code. It appears to want to convert everything to a Linux form. For example, directories are terminated with "/" without consideration for platform. On the other hand, paths are not always normalized when they are read in from APIs or from XML or script. Some functions appear to denormalize accidentally.

There are really two questions of normal form, one for directory termination and the other for platform-specific representation. Different functions produce different forms for paths, although all that I reviewed also guarded against unexpected forms. This leads to a lot of redundant checking, and terminating/unterminating, but doesn't appear broken. The same holds for platform normalization. However in this case the guards and reconversions don't keep up.

There is way to much code for me to make a thorough review, so I made a small number of changes to improve matters while keeping regression risk low. These are things that were already being done, just less consistently. So this is the basic idea:

  • Normalize all platform API result and user/external paths input.
  • Treat forward-slashing as the canonical separator.
  • Denormalize locally and only just prior to calling a platform API.
  • Output paths (file and directory) in canonical form.

This means that GSL can accept any input, and internal operations should be simpler (could be more so). But it also means that output will be consistent. This should help prevent the need for platform-specific code in scripts. It is a bit odd to see c:/foo/bar/ but certainly better than seeing c:\foo\bar/. Also windows compilers have no problem with forward-slashed paths, and they are definitely preferred (for cross compile).

There is also an aspect of the change that makes the directory input more friendly. These are now all valid directories:

  • foo
  • foo/
  • foo\ [windows only]

Previously only the last was valid on Windows and the first was not valid at all (terminating slash was expected).

hintjens added a commit that referenced this pull request Mar 5, 2015
Problem: inconsistent path treatment leads to confusing behavior.
@hintjens hintjens merged commit b6b52f9 into imatix:master Mar 5, 2015
@jschultz
Copy link
Contributor

jschultz commented Mar 5, 2015

ggfile.c (and ggfile.h) are generated from ggfile.gxl ('gsl ggfile.gxl') and should not be modified. It should be pretty easy to figure out how to modify ggfile.gxl in order to produce the desired change in ggfile.c.

Might be worth checking that this kind of thing hasn't happened before with other gxl files.

@evoskuil
Copy link
Member Author

evoskuil commented Mar 6, 2015

@jschultz Can't believe I missed that. I'll see if I can successfully retrofit the changes.

@jschultz
Copy link
Contributor

jschultz commented Mar 6, 2015

Shouldn't be difficult. Maybe those generated files can be make read-only? That is, all .c and .h files for which there exists a .gxl. Also all .d and .i files. Plus gsl.h, smt3.h, sfl.h

@evoskuil evoskuil deleted the normalize branch March 6, 2015 09:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants